Skip to content

Conversation

@rjalander
Copy link
Contributor

This PR is a continuation of #35, to implement other Core events

Copy link
Contributor

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!! This looks good.
Do we have anything to remove from the old implementation?

@rjalander
Copy link
Contributor Author

Thank you.
Yes, we need to remove the old implementation CDEventTypes.java and it's UT related files.

@afrittoli
Copy link
Contributor

Thank you. Yes, we need to remove the old implementation CDEventTypes.java and it's UT related files.

It would be nice to have it in this PR, I think, or do you plan on removing the old implementation all the end at once?
Since this is a new package it may be ok, as long is all happen this week.

@rjalander
Copy link
Contributor Author

Yes, we are still creating PRs for Source Code, Continues Integration and Deployment events to implement with this approach and planning to complete in couple of days.
I think I will create another PR once at the end.


cdEvent.setSubjectId("/dev/pipeline/run/subject");
cdEvent.setSubjectSource(URI.create("/dev/pipeline/run/subject"));
cdEvent.setSubjectSource(URI.create("http://dev/pipeline/run/subject"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: why http://? It's fine, but even the string starting with /dev should be a valid URI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to start with /dev

@afrittoli
Copy link
Contributor

Thank you!
It just came to mind that perhaps we should use the spec version v0.1.2 instead of v0.1.0 as starting point here since there were a couple of issues in the v0.1.0 version, the reason why we made the patch releases.

@rjalander
Copy link
Contributor Author

Ok, so we don't need to maintain a v0.1.0 version for Java-SDK and I can update this and other event PRs to use v0.1.2 from now?

@afrittoli
Copy link
Contributor

Ok, so we don't need to maintain a v0.1.0 version for Java-SDK and I can update this and other event PRs to use v0.1.2 from now?

Indeed, please use v0.1.2 - there are a few bug fixes that's good to have in.

@rjalander
Copy link
Contributor Author

Thank you @afrittoli, updated this PR to use 0.1.2 specification.

@rjalander
Copy link
Contributor Author

@afrittoli are we good to merge this PR now.

@afrittoli afrittoli merged commit 5d9b7b6 into cdevents:main May 3, 2023
@cdevents-bot cdevents-bot added the released Issue has been released label Jul 24, 2023
@cdevents-bot
Copy link
Collaborator

🎉 This issue has been resolved in v0.1.2 (Release Notes)

@cdevents-bot cdevents-bot added this to the v0.1.2 milestone Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

released Issue has been released

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants